Skip to content

chore(theme): migrate to NFS#2356

Open
ciiay wants to merge 8 commits intoredhat-developer:mainfrom
ciiay:rhidp-11832-migrate-theme-plugin-to-nfs
Open

chore(theme): migrate to NFS#2356
ciiay wants to merge 8 commits intoredhat-developer:mainfrom
ciiay:rhidp-11832-migrate-theme-plugin-to-nfs

Conversation

@ciiay
Copy link
Member

@ciiay ciiay commented Feb 19, 2026

Hey, I just made a Pull Request!

For RHIDP-11832

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or Updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)

Tests:

  1. Verified dev app with yarn start
    https://github.com/user-attachments/assets/847f7b95-a40f-44e2-a177-4ed3509f4ed5

  2. Verified dev legacy app with yarn start:legacy
    https://github.com/user-attachments/assets/e59b81d9-c89b-48a9-a8f3-4a9b6d2d6760

  3. Verified the exported theme plugin in RHDH locally
    In your_rhdh_path/packages/app/package.json change L49 to use your local rhdh-plugins theme package

    "@red-hat-developer-hub/backstage-plugin-theme": "portal:your_rhdh-plugins_path/workspaces/theme/plugins/theme",

and run yarn dev to start the app

rhidp_11832_rhdh_test.mp4

Signed-off-by: Yi Cai <yicai@redhat.com>
@ciiay ciiay force-pushed the rhidp-11832-migrate-theme-plugin-to-nfs branch from d73ba9b to b20c7dd Compare February 26, 2026 03:16
@rhdh-gh-app
Copy link

rhdh-gh-app bot commented Feb 26, 2026

Unexpected Changesets

The following changeset(s) reference packages that have not been changed in this PR:

  • /home/runner/work/rhdh-plugins/rhdh-plugins/workspaces/theme/.changeset/healthy-queens-mix.md: @red-hat-developer-hub/backstage-plugin-theme

Note that only changes that affect the published package require changesets, for example changes to tests and storybook stories do not require changesets.

Changed Packages

Package Name Package Path Changeset Bump Current Version
app-legacy workspaces/theme/packages/app-legacy none v0.0.0
app workspaces/theme/packages/app none v0.0.0
backend workspaces/theme/packages/backend none v0.0.0
@red-hat-developer-hub/backstage-plugin-bcc-test workspaces/theme/plugins/bcc-test patch v0.2.0
@red-hat-developer-hub/backstage-plugin-bui-test workspaces/theme/plugins/bui-test patch v0.2.0
@red-hat-developer-hub/backstage-plugin-mui4-test workspaces/theme/plugins/mui4-test patch v0.2.0
@red-hat-developer-hub/backstage-plugin-mui5-test workspaces/theme/plugins/mui5-test patch v0.2.0

Signed-off-by: Yi Cai <yicai@redhat.com>
Signed-off-by: Yi Cai <yicai@redhat.com>
Signed-off-by: Yi Cai <yicai@redhat.com>
Signed-off-by: Yi Cai <yicai@redhat.com>
Signed-off-by: Yi Cai <yicai@redhat.com>
Made-with: Cursor
@ciiay ciiay changed the title [WIP]chore(theme): migrate to NFS chore(theme): migrate to NFS Feb 28, 2026
@ciiay ciiay marked this pull request as ready for review February 28, 2026 02:36
@ciiay ciiay requested review from a team and logonoff as code owners February 28, 2026 02:36
@rhdh-qodo-merge
Copy link

rhdh-qodo-merge bot commented Feb 28, 2026

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Unauthenticated guest access

Description: The legacy app enables automatic guest sign-in (SignInPage with auto and
providers={['guest']}), which can unintentionally allow unauthenticated access in non-dev
deployments if not gated by environment/config and may expose internal Backstage pages and
metadata.
App.tsx [80-84]

Referred Code
  components: {
    SignInPage: props => <SignInPage {...props} auto providers={['guest']} />,
  },
  themes: getAllThemes(),
});
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
🔴
bindRoutes Component

Description:
bindRoutes({ bind }) { ... } is identical to existing route-binding blocks (e.g., Similar
Content 1). Consider reusing the existing module/function that defines these bindings (or
exporting a shared bindExternalRoutes helper) instead of duplicating the same bind(...)
calls.

PR Code:
App.tsx [63-79]

bindRoutes({ bind }) {
  bind(catalogPlugin.externalRoutes, {
    createComponent: scaffolderPlugin.routes.root,
    viewTechDoc: techdocsPlugin.routes.docRoot,
    createFromTemplate: scaffolderPlugin.routes.selectedTemplate,
  });
  bind(apiDocsPlugin.externalRoutes, {
    registerApi: catalogImportPlugin.routes.importPage,
  });
  bind(scaffolderPlugin.externalRoutes, {
    registerComponent: catalogImportPlugin.routes.importPage,
    viewTechDoc: techdocsPlugin.routes.docRoot,
  });
  bind(orgPlugin.externalRoutes, {
    catalogIndex: catalogPlugin.routes.catalogIndex,
  });
},

Codebase Context Code:
redhat-developer/rhdh-plugins/workspaces/dcm/packages/app/src/App.tsx [61-77]

 bindRoutes({ bind }) {
   bind(catalogPlugin.externalRoutes, {
     createComponent: scaffolderPlugin.routes.root,
     viewTechDoc: techdocsPlugin.routes.docRoot,
     createFromTemplate: scaffolderPlugin.routes.selectedTemplate,
   });
   bind(apiDocsPlugin.externalRoutes, {
     registerApi: catalogImportPlugin.routes.importPage,
   });
   bind(scaffolderPlugin.externalRoutes, {
     registerComponent: catalogImportPlugin.routes.importPage,
     viewTechDoc: techdocsPlugin.routes.docRoot,
   });
   bind(orgPlugin.externalRoutes, {
     catalogIndex: catalogPlugin.routes.catalogIndex,
   });
 },
SidebarLogo Component

Description:
SidebarLogo is duplicated verbatim (matches Similar Content 8 and several others). Reuse
the existing SidebarLogo component (export/import it) rather than redefining it in the PR.

PR Code:
Root.tsx [62-73]

const SidebarLogo = () => {
  const classes = useSidebarLogoStyles();
  const { isOpen } = useSidebarOpenState();

  return (
    <div className={classes.root}>
      <Link to="/" underline="none" className={classes.link} aria-label="Home">
        {isOpen ? <LogoFull /> : <LogoIcon />}
      </Link>
    </div>
  );
};

Codebase Context Code:
redhat-developer/rhdh-plugins/workspaces/konflux/packages/app/src/components/Root/Root.tsx [61-72]

const SidebarLogo = () => {
 const classes = useSidebarLogoStyles();
 const { isOpen } = useSidebarOpenState();

 return (
   <div className={classes.root}>
     <Link to="/" underline="none" className={classes.link} aria-label="Home">
       {isOpen ? <LogoFull /> : <LogoIcon />}
     </Link>
   </div>
 );
};
Root Component

Description:
The Root sidebar layout shown matches the beginning portion of the existing Root
implementation (Similar Content 16) up to the visible scope boundary. Prefer importing and
extending the existing Root (or extracting a shared AppSidebar component) instead of
duplicating the same sidebar structure and items.

PR Code:
Root.tsx [75-117]

export const Root = ({ children }: PropsWithChildren<{}>) => (
  <SidebarPage>
    <Sidebar>
      <SidebarLogo />
      <SidebarGroup label="Search" icon={<SearchIcon />} to="/search">
        <SidebarSearchModal />
      </SidebarGroup>
      <SidebarDivider />
      <SidebarGroup label="Menu" icon={<MenuIcon />}>
        {/* Global nav, not org-specific */}
        <SidebarItem icon={HomeIcon} to="catalog" text="Home" />
        <MyGroupsSidebarItem
          singularTitle="My Group"
          pluralTitle="My Groups"
          icon={GroupIcon}
        />
        <SidebarItem icon={ExtensionIcon} to="api-docs" text="APIs" />
        <SidebarItem icon={LibraryBooks} to="docs" text="Docs" />
        <SidebarItem icon={CreateComponentIcon} to="create" text="Create..." />
        {/* End global nav */}
        <SidebarDivider />


 ... (clipped 22 lines)

Codebase Context Code:
redhat-developer/rhdh-plugins/workspaces/konflux/packages/app/src/components/Root/Root.tsx [74-111]

export const Root = ({ children }: PropsWithChildren<{}>) => (
 <SidebarPage>
   <Sidebar>
     <SidebarLogo />
     <SidebarGroup label="Search" icon={<SearchIcon />} to="/search">
       <SidebarSearchModal />
     </SidebarGroup>
     <SidebarDivider />
     <SidebarGroup label="Menu" icon={<MenuIcon />}>
       {/* Global nav, not org-specific */}
       <SidebarItem icon={HomeIcon} to="catalog" text="Home" />
       <MyGroupsSidebarItem
         singularTitle="My Group"
         pluralTitle="My Groups"
         icon={GroupIcon}
       />
       <SidebarItem icon={ExtensionIcon} to="api-docs" text="APIs" />
       <SidebarItem icon={LibraryBooks} to="docs" text="Docs" />
       <SidebarItem icon={CreateComponentIcon} to="create" text="Create..." />
       {/* End global nav */}


... (clipped 18 lines)
Custom Compliance
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Incomplete diff coverage: Several added/modified files were not included in the provided diff, so it cannot be
verified whether any newly introduced critical actions require audit logging and have the
required context.

Referred Code
/*
 * Copyright Red Hat, Inc.

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status:
Incomplete diff coverage: Because multiple changed files were omitted from the diff, naming clarity and identifier
intent across the newly added NFS/legacy app code cannot be fully validated.

Referred Code
/*
 * Copyright Red Hat, Inc.

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Incomplete diff coverage: Newly added/modified application entrypoints and modules were not fully visible in the
diff, preventing verification that potential failure points include robust error handling
and edge case management.

Referred Code
/*
 * Copyright Red Hat, Inc.

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status:
Incomplete diff coverage: The diff does not include all new/updated UI and routing code paths, so it cannot be
confirmed that any user-facing errors introduced remain generic and do not expose internal
details.

Referred Code
/*
 * Copyright Red Hat, Inc.

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status:
Incomplete diff coverage: With several application files omitted from the diff, it cannot be verified whether new
logging was introduced and, if so, whether it is structured and free of sensitive data.

Referred Code
"@backstage/backend-defaults": "^0.14.0",
"@backstage/config": "^1.3.6",

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Incomplete diff coverage: Search/navigation and other user-input-related components were not fully included in the
diff, so it cannot be confirmed that any newly introduced external inputs are
validated/sanitized and handled securely.

Referred Code
/*
 * Copyright Red Hat, Inc.

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@rhdh-qodo-merge
Copy link

rhdh-qodo-merge bot commented Feb 28, 2026

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Security
Restore missing permission check

Restore the missing RequirePermission check for the /catalog-import route to
prevent a critical security vulnerability that allows unauthorized access.

workspaces/theme/packages/app/src/App.tsx [130-160]

 <AppRouter>
   <div>
     <FlatRoutes>
       <Route path="/catalog" element={<CatalogIndexPage />} />
       <Route
         path="/catalog/:namespace/:kind/:name"
         element={<CatalogEntityPage />}
       >
         {entityPage}
       </Route>
       <Route path="/docs" element={<TechDocsIndexPage />} />
       <Route
         path="/docs/:namespace/:kind/:name/*"
         element={<TechDocsReaderPage />}
       >
         <TechDocsAddons>
           <ReportIssue />
         </TechDocsAddons>
       </Route>
       <Route path="/create" element={<ScaffolderPage />} />
       <Route path="/api-docs" element={<ApiExplorerPage />} />
       {/* Top-level element must be a plugin component for convertLegacyApp */}
-      <Route path="/catalog-import" element={<CatalogImportPage />} />
+      <Route
+        path="/catalog-import"
+        element={
+          <RequirePermission permission={catalogEntityCreatePermission}>
+            <CatalogImportPage />
+          </RequirePermission>
+        }
+      />
       <Route path="/search" element={<SearchPage />}>
         {searchPage}
       </Route>
       <Route path="/settings" element={<UserSettingsPage />} />
       <Route path="/catalog-graph" element={<CatalogGraphPage />} />
     </FlatRoutes>
   </div>
 </AppRouter>
  • Apply / Chat
Suggestion importance[1-10]: 10

__

Why: The suggestion correctly identifies a critical security vulnerability where a permission check on the /catalog-import route was removed during refactoring, which would allow unauthorized access.

High
High-level
Avoid creating a separate legacy app

Instead of creating a parallel app-legacy package, perform the New Frontend
System (NFS) migration within the existing app package. Use the provided
compatibility APIs for a smoother, incremental transition and to avoid the
maintenance overhead of a duplicated application.

Examples:

workspaces/theme/packages/app-legacy/package.json [1-85]
{
  "name": "app-legacy",
  "version": "0.0.0",
  "private": true,
  "bundled": true,
  "repository": {
    "type": "git",
    "url": "https://github.com/redhat-developer/rhdh-plugins",
    "directory": "workspaces/theme/packages/app-legacy"
  },

 ... (clipped 75 lines)
workspaces/theme/packages/app-legacy/src/App.tsx [1-135]
/*
 * Copyright Red Hat, Inc.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software

 ... (clipped 125 lines)

Solution Walkthrough:

Before:

// File structure has two separate apps
/packages/
  app/
    - src/App.tsx (New NFS implementation)
  app-legacy/
    - src/App.tsx (Old legacy implementation)

// workspaces/theme/packages/app/src/App.tsx
import { createApp } from '@backstage/frontend-defaults';
import { convertLegacyApp } from '@backstage/core-compat-api';
// ...
const app = createApp({
  features: [
    ...convertLegacyApp(legacyRootElement),
    // ... other NFS features
  ],
});
export default app.createRoot();

// workspaces/theme/packages/app-legacy/src/App.tsx (new file)
import { createApp } from '@backstage/app-defaults';
const app = createApp({ apis, themes, ... });
export default app.createRoot(<Root>{routes}</Root>);

After:

// File structure has only one app
/packages/
  app/
    - src/App.tsx (Hybrid NFS/Legacy implementation)
  // app-legacy/ directory is removed

// workspaces/theme/packages/app/src/App.tsx
// The migration is done within the single 'app' package,
// using compatibility APIs to bridge legacy and NFS features
// without duplicating the entire application.

import { createApp } from '@backstage/frontend-defaults';
import { convertLegacyApp } from '@backstage/core-compat-api';
// ...

const legacyRootElement = (
  <AppRouter>
    {/* All legacy routes and components are defined here */}
  </AppRouter>
);

const app = createApp({
  features: [
    // Convert all legacy routes and plugins to NFS features
    ...convertLegacyApp(legacyRootElement),
    // New NFS-native features can be added here
  ],
});
export default app.createRoot();
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a major architectural issue—duplicating the entire application into an app-legacy package—which significantly increases long-term maintenance complexity, and it proposes a better, standard approach.

High
Possible issue
Use absolute paths for navigation

Update the to prop in SidebarItem components to use absolute paths (e.g.,
/api-docs) for consistent and reliable navigation.

workspaces/theme/packages/app/src/components/Root/Root.tsx [90-92]

-<SidebarItem icon={ExtensionIcon} to="api-docs" text="APIs" />
-<SidebarItem icon={LibraryBooks} to="docs" text="Docs" />
-<SidebarItem icon={CreateComponentIcon} to="create" text="Create..." />
+<SidebarItem icon={ExtensionIcon} to="/api-docs" text="APIs" />
+<SidebarItem icon={LibraryBooks} to="/docs" text="Docs" />
+<SidebarItem icon={CreateComponentIcon} to="/create" text="Create..." />
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: This suggestion corrects an inconsistency introduced in the PR, ensuring all navigation links use absolute paths to prevent potential routing bugs.

Medium
Add null check for root element

Add a null check for the root DOM element before rendering the React app to
prevent potential startup crashes.

workspaces/theme/packages/app-legacy/src/index.tsx [22]

-ReactDOM.createRoot(document.getElementById('root')!).render(<App />);
+const rootElement = document.getElementById('root');
+if (!rootElement) {
+  throw new Error('Failed to find the root element');
+}
+ReactDOM.createRoot(rootElement).render(<App />);
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion improves code robustness by replacing a non-null assertion with an explicit check, preventing a potential runtime crash if the root element is missing.

Medium
Fix style import path

Update the import path for makeStyles from @material-ui/core to
@material-ui/core/styles.

workspaces/theme/packages/app/src/modules/nav/LogoFull.tsx [16]

-import { makeStyles } from '@material-ui/core';
+import { makeStyles } from '@material-ui/core/styles';
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: This is a correct suggestion that aligns with Material-UI's best practices, improving code maintainability and preventing potential bundling issues.

Low
General
Add svg accessibility attributes

Improve SVG accessibility by adding a role="img" attribute, a <title> element, and an
aria-labelledby attribute to the logo.

workspaces/theme/packages/app/src/modules/nav/LogoFull.tsx [32-41]

 <svg
   className={classes.svg}
   xmlns="http://www.w3.org/2000/svg"
   viewBox="0 0 2079.95 456.05"
+  role="img"
+  aria-labelledby="logoFullTitle"
 >
+  <title id="logoFullTitle">Application Full Logo</title>
   <path
     className={classes.path}
     d="M302.9,180a80.62,80.62,0,0,0,13.44-10.37c..."
   />
 </svg>
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a missing accessibility feature and provides a standard way to make the SVG logo accessible to screen reader users, which is an important improvement.

Medium
Use JSS for dynamic styling

Refactor the component to use JSS (makeStyles) for dynamic styling based on the
isOpen state, instead of using inefficient and conflict-prone inline styles.

workspaces/theme/packages/app/src/modules/nav/SidebarLogo.tsx [40-60]

 export const SidebarLogo = () => {
-  const classes = useSidebarLogoStyles();
   const { isOpen } = useSidebarOpenState();
-  const sidebarWidth = isOpen
-    ? sidebarConfig.drawerWidthOpen
-    : sidebarConfig.drawerWidthClosed;
+  const classes = useSidebarLogoStyles({ isOpen });
 
   return (
-    <div className={classes.root} style={{ width: sidebarWidth }}>
-      <Link
-        to="/"
-        underline="none"
-        className={classes.link}
-        aria-label="Home"
-        style={{ width: sidebarWidth }}
-      >
+    <div className={classes.root}>
+      <Link to="/" underline="none" className={classes.link} aria-label="Home">
         {isOpen ? <LogoFull /> : <LogoIcon />}
       </Link>
     </div>
   );
 };
  • Apply / Chat
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly points out the use of inline styles and proposes a better practice using JSS for dynamic styling, which improves code quality and maintainability.

Low
  • Update

Comment on lines +2 to +6
'@red-hat-developer-hub/backstage-plugin-mui4-test': patch
'@red-hat-developer-hub/backstage-plugin-mui5-test': patch
'@red-hat-developer-hub/backstage-plugin-bcc-test': patch
'@red-hat-developer-hub/backstage-plugin-bui-test': patch
'@red-hat-developer-hub/backstage-plugin-theme': patch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be at least a minor bump 😆

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey Jackson, thanks for the prompt review! I thought the same way but I referred the BCP pr here
https://github.com/backstage/community-plugins/pull/7567/changes#diff-8e18ff939a9d6771baeba4b4972ebece660a3f1e4936726a02672912a141cb79

Let's see what other reviewers have to say, I can update it later 🤝

Signed-off-by: Yi Cai <yicai@redhat.com>
@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
26.7% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants